-
-
Notifications
You must be signed in to change notification settings - Fork 667
fix 17868 - add pragma(crt_con-/destructor) #7182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks for your pull request, @MartinNowak! Bugzilla references
|
6fed4d0 to
5522b95
Compare
a93389a to
4f9e3ba
Compare
|
I'm okay with this approach as opposed to clever games with |
|
This tool is useful despite |
changelog/crt-constructor.dd
Outdated
|
|
||
| $(RED Note:) The order in which constructors are executed is unspecified. | ||
|
|
||
| ---- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be three dashes? Or is it three or more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 or more works
|
Perhaps a test that attaches the pragam to something else than a function. |
ca1e93e to
03b281a
Compare
Done |
|
What about static methods in a class or struct? |
Works, this is a low-level tool (just as |
|
It would be nice with a test for that. |
It's too low-level, I wouldn't want to promise that for all ABIs, use at your own risk. |
A short list of pros and cons of the approaches woudl be very helpful. |
|
This PR is not about |
|
Ping @WalterBright, @andralex. Anything preventing us from adding that feature for low-level runtime hackers and C++ interop? |
|
@WalterBright I'm good to go with this, will approve. |
src/ddmd/dsymbolsem.d
Outdated
| else if (pd.ident == Id.crt_constructor || pd.ident == Id.crt_destructor) | ||
| { | ||
| if (pd.args && pd.args.dim != 0) | ||
| pd.error("too many arguments"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too many is misleading. Should be arguments are not allowed for crt_constructor and crt_destructor functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions should also be forced to use C linkage.
|
Thank you @MartinNowak for doing this work, especially the fixing of the My complaint centers around using an awkward pragma to do the dirty work. It has the effect of putting every function in its scope into the initializer list, which is a bit bizarre as I can't imagine code needing that. A better placement for the pragma would be inside the function body, which would avoid the need for the The complaints around Another issue is if one is not using -betterC, but wishes to install a function into the C ctor/dtor list. One way is for the user to put such code into a separate file, compile that file with -betterC, and link it in. Another way is to do this: This also avoids the need for the |
|
That's mostly because it's a function attribute (pragma) everywhere else, e.g. gdc/ldc/GCC/clang. The recursion was added to deal with things like the following and maybe template functions. pragma(crt_constructor)
version (linux) void init() {}I could instead change it to check that it directly applies to a function. |
13434c2 to
67a59cd
Compare
|
Do we have a precedent of a pragma only applicable to one declaration? If not, would an attribute a la |
|
Is it really a function attribute? It doesn't affect the function signature or ABI. Given the salad of attributes we have, adding more kinda scares me, especially for one so rarely used as this one. Recall how C++'s
I don't find the recursive example compelling enough to have to explain why it doesn't affect aggregate member functions. Let's pick something that will be easy to document. |
Why do you think a function attribute does have to affect the signature or API? We also have UDAs and these are simply defined as compile time only meta-data for certain declarations. Recognize some special cases in the compiler and they are a perfect fit for such situations. LDC and GDC already do this for years: The mangle pragma is also a perfect fit for such an UDA. I really don't understand why DMD doesn't just finally follow the other compilers lead here to introduce a // If you don't want the import in user code, add a public import to object.d instead
import dmd.attribute;
@crt_constructor void foo() {}
@crt_destructor @mangle("some_mangled_name") void bar() {}You even get compile-time reflection to detect whether any such attribute is applied to a function for free, there's no global namespace pollution, and there are many more benefits. We have this nice UDA feature, why not use it? |
We already have [1] https://github.com/dlang/druntime/blob/master/src/core/attribute.d |
|
Can we please keep the attribute discussion off here, it's a necessary, but separate discussion. So far similar functionality was implemented as pragmas, and there isn't even functionality to recognize special attributes in dmd yet. It's also a pragma declaration in LDC. BTW, that's the second time this review takes a detour into an slightly OT discussion.
That's hardly feasible as it required changing build tools to compile one module with different flags. It also changes the other semantics of that module. |
- allows to run con-/destructors before/after CRT startup/shutdown - primary use-case is implementing modular startup in druntime itself
This PR has already been updated to handle this in the same way as |
FWIW in LDC we have |
Yes, as mentioned in #7182 (comment). Can we please settle the bike-shedding on pragma vs. attributes vs. pragma with function name. This PR is implemented just like pragma mangle, and now doesn't allow more than one function. |
Does it need to be? it would be nice to drop LDC's specific ones and use the dmd ones (LDCs is also a block pragma but that is less of an issue). Having said that I'm not sure how widely they are used. CRT constructors are rather rare and specifying a priority is even more rare. Perhaps you could make the priority optional and implementation defined? |
|
No, we went through this, LDC/GDC can keep their pragmas with priority support and map the one added in this PR to the default priority. As you said there is hardly any use-case for priorities to begin with, and even with ldc/gdc priorities were not global, but local (translation unit). |
|
Fair enough. |
|
@MartinNowak now when this is merged, how does this related to the [1] Line 3372 in 901fde2
|
|
crt_constructor is not allow to set static immutable. static immutable int my_i ;
int test(){
return 3;
}
shared static this(){
my_i = test(); // work
}
pragma(crt_constructor) extern(C) void init1(){
my_i = test(); // Error: cannot modify immutable expression my_i
} |
|
@changloong You could raise a bug on that instead of commenting here. |
|
@jacob-carlborg - I will assume no, because it needs to be emitted into every DSO. |
|
@ibuclaw thanks for tip |
|
Thanks for this killer feature for betteC programs. Tested with mir-cpuid, DMD segfaults if |
|
Bit late, but specs PR is here: dlang/dlang.org#2757 |
required for #6956